Skip to content

builds: add feature-gated ephemeral ext4 BuildKit root volume - #332

Closed
rgarcia wants to merge 5 commits into
hypeship/buildkit-mounted-rootfrom
hypeship/buildkit-ephemeral-volume
Closed

builds: add feature-gated ephemeral ext4 BuildKit root volume#332
rgarcia wants to merge 5 commits into
hypeship/buildkit-mounted-rootfrom
hypeship/buildkit-ephemeral-volume

Conversation

@rgarcia

@rgarcia rgarcia commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Summary

When build.disk_root.enabled is set, the build manager creates a fixed-size ext4 volume per build (build-disk-<id>, size from build.disk_root.size_gb, default 20 GB), attaches it to the builder VM at /var/lib/buildkit, and deletes it with the builder VM. The builder agent already detects that mountpoint and uses it directly (see the PR below in this stack), so the guest skips its tmpfs fallback.

When disabled (the default), no volume is created and the existing tmpfs path is unchanged.

Changes

  • builds.Config: new DiskRootEnabled / DiskRootSizeGB fields; buildkitRootMountPath and DefaultDiskRootSizeGB constants.
  • executeBuild: setupDiskRootVolume creates the volume when enabled (error fails the build before instance creation); builderVolumeAttachments builds the attachment list, appending the rw /var/lib/buildkit mount only when a disk root volume exists. Volume deletion is deferred alongside the existing source/config volume cleanup.
  • setupDiskRootVolume tolerates a leftover volume from a crashed build attempt: the deterministic build-disk-<id> is deleted and recreated instead of permanently failing the recovered build on ErrAlreadyExists.
  • Config plumbing: build.disk_root.enabled / size_gb in cmd/api/config (with validation), lib/providers, and config.example.yaml.

Tests

lib/builds/disk_root_test.go (no privileged mounts; mock instance/volume managers):

  • disabled: no volume created, default 2-attachment path preserved
  • enabled: default vs configured size, ID format, create-error propagation (build fails before instance creation)
  • leftover volume from a crashed attempt: deleted and recreated; delete failure fails the build
  • end-to-end executeBuild lifecycle: volume created, attached at /var/lib/buildkit, deleted when the build finishes
  • attachment list shape for both modes

go test ./lib/builds/... ./cmd/api/config/... ./lib/providers/... and go build ./... pass.


Note

Medium Risk
Changes build VM volume lifecycle and host disk use per build when enabled; default-off and well-tested, but recovery deletes stale builder instances and widens mount rules for internal creates only.

Overview
Adds optional build.disk_root config so each source build can get a dedicated ext4 volume (build-disk-<id>) mounted at /var/lib/buildkit on the builder VM instead of tmpfs, with size from size_gb (default 20 when unset). When disabled, behavior is unchanged.

executeBuild creates the volume before the builder instance (failure aborts before VM create), attaches it via builderVolumeAttachments, and defers deletion with the other build volumes. setupDiskRootVolume handles ErrAlreadyExists from crashed runs by deleting leftovers; if delete hits ErrInUse, it removes a stale builder-<id> instance and retries.

Instance validation gains internal-only AllowSystemVolumeMounts so builder VMs may mount exactly /var/lib/buildkit (not exposed from the API). Config disk creation now uses per-call unique temp directories to avoid path collisions under shared TMPDIR.

Tests cover disabled/enabled paths, crash recovery, and full executeBuild lifecycle with mocks.

Reviewed by Cursor Bugbot for commit bba318a. Bugbot is set up for automated code reviews on this repo. Configure here.

rgarcia added 2 commits August 2, 2026 02:53
When build.disk_root.enabled is set, the build manager creates a
fixed-size ext4 volume per build, attaches it to the builder VM at
/var/lib/buildkit, and deletes it with the builder VM. The builder
agent already detects the mount and uses it directly; when disabled,
the tmpfs fallback path is unchanged.
A host crash mid-build leaked the deterministic build-disk-<id> volume;
on recovery, setupDiskRootVolume failed on ErrAlreadyExists and the
recovered build could never run. Delete the leftover and create a fresh
volume instead.
@rgarcia
rgarcia marked this pull request as ready for review August 2, 2026 04:13

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.

Autofix Details

Bugbot Autofix prepared fixes for both issues found in the latest run.

  • ✅ Fixed: BuildKit mount blocked by validation
    • I added a narrow validation exception for /var/lib/buildkit so disk-root builders can attach the BuildKit volume while other /var mounts remain blocked.
  • ✅ Fixed: Leftover disk delete when attached
    • On leftover disk ErrInUse, the build manager now deletes the stale recorded builder instance, clears its metadata, retries volume deletion, and then recreates the disk volume.

Create PR

Or push these changes by commenting:

@cursor push 411978317b
Preview (411978317b)
diff --git a/lib/builds/disk_root_test.go b/lib/builds/disk_root_test.go
--- a/lib/builds/disk_root_test.go
+++ b/lib/builds/disk_root_test.go
@@ -120,6 +120,60 @@
 	assert.Empty(t, volID)
 }
 
+func TestSetupDiskRootVolume_LeftoverInUseCleansStaleBuilder(t *testing.T) {
+	mgr, instanceMgr, volumeMgr, tempDir := setupTestManager(t)
+	defer os.RemoveAll(tempDir)
+	mgr.config.DiskRootEnabled = true
+
+	buildID := "build-1"
+	staleBuilderID := "inst-builder-build-1"
+	meta := &buildMetadata{
+		ID:              buildID,
+		Status:          StatusBuilding,
+		Request:         &CreateBuildRequest{Dockerfile: "FROM alpine"},
+		CreatedAt:       time.Now(),
+		BuilderInstance: &staleBuilderID,
+	}
+	require.NoError(t, writeMetadata(mgr.paths, meta))
+	instanceMgr.instances[staleBuilderID] = &instances.Instance{
+		StoredMetadata: instances.StoredMetadata{
+			Id:   staleBuilderID,
+			Name: "builder-build-1",
+		},
+		State: instances.StateRunning,
+	}
+
+	created := false
+	volumeMgr.createFunc = func(ctx context.Context, req volumes.CreateVolumeRequest) (*volumes.Volume, error) {
+		if !created {
+			created = true
+			return nil, volumes.ErrAlreadyExists
+		}
+		return &volumes.Volume{Id: *req.Id, Name: req.Name, SizeGb: req.SizeGb}, nil
+	}
+
+	deleteAttempts := 0
+	volumeMgr.deleteFunc = func(ctx context.Context, id string) error {
+		deleteAttempts++
+		if deleteAttempts == 1 {
+			return volumes.ErrInUse
+		}
+		return nil
+	}
+
+	volID, err := mgr.setupDiskRootVolume(context.Background(), buildID)
+
+	require.NoError(t, err)
+	assert.Equal(t, "build-disk-build-1", volID)
+	assert.Equal(t, 1, instanceMgr.deleteCallCount)
+	assert.Equal(t, 2, deleteAttempts)
+	assert.Equal(t, 2, volumeMgr.createCallCount)
+
+	metaAfter, err := readMetadata(mgr.paths, buildID)
+	require.NoError(t, err)
+	assert.Nil(t, metaAfter.BuilderInstance)
+}
+
 func TestBuilderVolumeAttachments(t *testing.T) {
 	attachments := builderVolumeAttachments("src-vol", "cfg-vol", "")
 	require.Len(t, attachments, 2)

diff --git a/lib/builds/manager.go b/lib/builds/manager.go
--- a/lib/builds/manager.go
+++ b/lib/builds/manager.go
@@ -803,7 +803,16 @@
 	if errors.Is(err, volumes.ErrAlreadyExists) {
 		// A previous attempt at this build crashed before cleanup. Delete
 		// the leftover volume and start fresh.
-		if delErr := m.volumeManager.DeleteVolume(ctx, volID); delErr != nil {
+		delErr := m.volumeManager.DeleteVolume(ctx, volID)
+		if errors.Is(delErr, volumes.ErrInUse) {
+			// The leftover volume may still be attached to a stale builder
+			// instance from a crashed process. Remove it and retry.
+			if cleanupErr := m.cleanupStaleBuilderInstance(ctx, buildID); cleanupErr != nil {
+				return "", fmt.Errorf("cleanup stale builder instance: %w", cleanupErr)
+			}
+			delErr = m.volumeManager.DeleteVolume(ctx, volID)
+		}
+		if delErr != nil {
 			return "", fmt.Errorf("delete leftover buildkit root volume: %w", delErr)
 		}
 		_, err = m.volumeManager.CreateVolume(ctx, volumes.CreateVolumeRequest{
@@ -818,6 +827,28 @@
 	return volID, nil
 }
 
+func (m *manager) cleanupStaleBuilderInstance(ctx context.Context, buildID string) error {
+	meta, err := readMetadata(m.paths, buildID)
+	if err != nil {
+		return fmt.Errorf("read build metadata: %w", err)
+	}
+	if meta.BuilderInstance == nil || *meta.BuilderInstance == "" {
+		return nil
+	}
+
+	builderInstanceID := *meta.BuilderInstance
+	if err := m.instanceManager.DeleteInstance(ctx, builderInstanceID); err != nil && !errors.Is(err, instances.ErrNotFound) {
+		return fmt.Errorf("delete stale builder instance %s: %w", builderInstanceID, err)
+	}
+
+	meta.BuilderInstance = nil
+	if err := writeMetadata(m.paths, meta); err != nil {
+		return fmt.Errorf("clear stale builder instance metadata: %w", err)
+	}
+
+	return nil
+}
+
 // builderVolumeAttachments returns the volume attachments for a builder VM.
 // diskRootVolID is empty when the disk root feature is disabled.
 func builderVolumeAttachments(sourceVolID, configVolID, diskRootVolID string) []instances.VolumeAttachment {

diff --git a/lib/instances/create.go b/lib/instances/create.go
--- a/lib/instances/create.go
+++ b/lib/instances/create.go
@@ -51,6 +51,12 @@
 	"/var",
 }
 
+// allowedSystemMountPaths are explicit exceptions under system directories that
+// are required for internal platform workloads.
+var allowedSystemMountPaths = map[string]struct{}{
+	"/var/lib/buildkit": {},
+}
+
 // generateVsockCID converts first 8 chars of instance ID to a unique CID
 // CIDs 0-2 are reserved (hypervisor, loopback, host)
 // Returns value in range 3 to 4294967295
@@ -665,7 +671,7 @@
 		cleanPath := filepath.Clean(vol.MountPath)
 
 		// Check for system directories
-		if isSystemDirectory(cleanPath) {
+		if isSystemDirectory(cleanPath) && !isAllowedSystemMountPath(cleanPath) {
 			return fmt.Errorf("volume %s: cannot mount to system directory %q", vol.VolumeID, cleanPath)
 		}
 
@@ -704,6 +710,11 @@
 	return false
 }
 
+func isAllowedSystemMountPath(path string) bool {
+	_, ok := allowedSystemMountPaths[filepath.Clean(path)]
+	return ok
+}
+
 // startAndBootVM starts the VMM and boots the VM
 func (m *manager) startAndBootVM(
 	ctx context.Context,

diff --git a/lib/instances/resource_limits_test.go b/lib/instances/resource_limits_test.go
--- a/lib/instances/resource_limits_test.go
+++ b/lib/instances/resource_limits_test.go
@@ -44,6 +44,29 @@
 	assert.Contains(t, err.Error(), "system directory")
 }
 
+func TestValidateVolumeAttachments_BuildkitRootSystemPathAllowed(t *testing.T) {
+	t.Parallel()
+	volumes := []VolumeAttachment{{
+		VolumeID:  "vol-1",
+		MountPath: "/var/lib/buildkit",
+	}}
+
+	err := validateVolumeAttachments(volumes)
+	assert.NoError(t, err)
+}
+
+func TestValidateVolumeAttachments_VarSubdirectoryStillBlocked(t *testing.T) {
+	t.Parallel()
+	volumes := []VolumeAttachment{{
+		VolumeID:  "vol-1",
+		MountPath: "/var/lib/other",
+	}}
+
+	err := validateVolumeAttachments(volumes)
+	assert.Error(t, err)
+	assert.Contains(t, err.Error(), "system directory")
+}
+
 func TestValidateVolumeAttachments_DuplicatePaths(t *testing.T) {
 	t.Parallel()
 	volumes := []VolumeAttachment{

You can send follow-ups to the cloud agent here.

Comment thread lib/builds/manager.go
Comment thread lib/builds/manager.go
rgarcia added 2 commits August 2, 2026 04:43
- Allow the exact /var/lib/buildkit mount path through volume attachment
  validation so builder creation succeeds with the disk root feature.
- When a leftover buildkit root volume is still attached to a surviving
  builder VM, delete the stale builder (detaching the volume) and retry
  the delete instead of failing recovery on ErrInUse.
- Write the build config ext4 disk to a unique per-call temp directory
  instead of a fixed TMPDIR path that concurrent builds and parallel
  test runs can collide on.
The exact-path exemption applied to every instance creation, so any API
caller could mount a volume at /var/lib/buildkit in their own VM. Gate it
behind AllowSystemVolumeMounts on the domain create request, which is
never populated from API requests; only the builds manager sets it when
creating builder VMs.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Wrong error wrapped on lookup
    • Updated the stale-builder lookup failure path to wrap getErr instead of the prior volume ErrInUse, preserving the correct error chain.

Create PR

Or push these changes by commenting:

@cursor push c5e6c475b9
Preview (c5e6c475b9)
diff --git a/lib/builds/manager.go b/lib/builds/manager.go
--- a/lib/builds/manager.go
+++ b/lib/builds/manager.go
@@ -835,7 +835,7 @@
 	builderName := fmt.Sprintf("builder-%s", buildID)
 	inst, getErr := m.instanceManager.GetInstance(ctx, builderName)
 	if getErr != nil {
-		return fmt.Errorf("leftover buildkit root volume still attached and stale builder %q not found: %w", builderName, err)
+		return fmt.Errorf("leftover buildkit root volume still attached and stale builder %q not found: %w", builderName, getErr)
 	}
 	if delErr := m.instanceManager.DeleteInstance(ctx, inst.Id); delErr != nil {
 		return fmt.Errorf("delete stale builder %s holding leftover buildkit root volume: %w", inst.Id, delErr)

You can send follow-ups to the cloud agent here.

Reviewed by Cursor Bugbot for commit 4654ffa. Configure here.

Comment thread lib/builds/manager.go Outdated
@rgarcia
rgarcia force-pushed the hypeship/buildkit-ephemeral-volume branch from bba318a to 0aa1048 Compare August 2, 2026 12:48
@rgarcia
rgarcia force-pushed the hypeship/buildkit-mounted-root branch from 7114b82 to b66a729 Compare August 3, 2026 17:50
@rgarcia
rgarcia force-pushed the hypeship/buildkit-ephemeral-volume branch from 9844255 to bba318a Compare August 3, 2026 17:50
@rgarcia

rgarcia commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Closing in favor of a fresh first-class Builder resource stack. Useful implementation and test work from this PR will be selectively reapplied in smaller reviewable layers.

@rgarcia rgarcia closed this Aug 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants